Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Screen reader support #1182

Merged
merged 56 commits into from
Feb 15, 2018
Merged

Screen reader support #1182

merged 56 commits into from
Feb 15, 2018

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jan 3, 2018

FYI @jluk

This PR adds support for screen readers (targeting NVDA on Windows initially). Screen reader support is disabled by default and must be opt in (screenReaderMode setting), this is so we can avoid all the DOM manipulation when we don't need it for performance reasons.

Currently supported

  • Terminal is announced when focused
  • Characters entered into the terminal via keyboard are announced
    • Character and word echo settings are respected
  • Read command output upon execution (up to 20 rows to prevent DOM from growing)
  • Support screen reader navigation within current viewport
    • NVDA can read out row content on mouse over
    • NVDA/VoiceOver navigation mode works with rows

Known issues

  • Characters are not read out when deleted from the prompt
  • Reading out history when navigating shells using up/down sometimes doesn't work
  • Holding down up/down while navigating rows in NVDA can skip over the terminal

Part of #731

@Tyriar Tyriar added this to the 3.1.0 milestone Jan 3, 2018
@Tyriar Tyriar self-assigned this Jan 3, 2018
@ahicks92
Copy link

ahicks92 commented Jan 3, 2018

Does this respect my keyboard echo settings? If not, that's amazingly annoying.

I mention it because there is an anti-pattern in i.e. google docs where you get the screen readers to read via echoing to a live region, and this both doesn't respect settings and doesn't work out with punctuation (some is silent because it's trying to read it as words).

As someone who very intentionally doesn't use character or word echo, being forced into it would be suboptimal. It's distracting and not very useful when i.e. on conference calls or whatever, because either it's pulling you away from whatever else is important or everyone else has to hear it if you're unlucky enough not to be able to have headphones.

@Tyriar
Copy link
Member Author

Tyriar commented Jan 4, 2018

@camlorn character echo settings are respected, word echo does not work currently but it could potentially later on (I made a note in the PR).

The live region is used to read out the command output, if you have anything I need to watch out for here I'd love to hear it. The live region seemed to handle perfectly what I wanted with the web APIs I have access to; to allow reading out of a stream of text (that may be incomplete) and stop the screen reader upon typing or when there is too much output.

@Tyriar Tyriar changed the base branch from v3 to master January 5, 2018 16:21
@Tyriar
Copy link
Member Author

Tyriar commented Jan 28, 2018

I was planning on allowing calls to scrollPage/scrollToTop/scrollToBottom to scroll and focus in navigation mode, unfortunately screen readers take complete control of the keyboard in these modes and I cannot intercept the needed keyboard strokes.

In case it's useful later, here's the commit b138264

@Tyriar Tyriar removed the work-in-progress Do not merge label Jan 28, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Jan 28, 2018

This is ready for review

@Tyriar Tyriar modified the milestones: 3.1.0, 3.2.0 Jan 28, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Jan 28, 2018

Pushing to v3.2.0 as this is a big PR and we have a bunch out for review in v3.1.0.

Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🍣

@Tyriar Tyriar merged commit 93e5996 into xtermjs:master Feb 15, 2018
@Tyriar Tyriar deleted the 731_screen_reader branch February 15, 2018 14:20
@Tyriar Tyriar mentioned this pull request Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants